Skip to content
This repository has been archived by the owner on Jun 9, 2024. It is now read-only.

refactor(blockchain): Minor cleanup and add safety checks. #1266

Merged
merged 16 commits into from
Nov 1, 2023

Conversation

itsdevbear
Copy link
Contributor

@itsdevbear itsdevbear commented Nov 1, 2023

Summary by CodeRabbit

Refactor:

  • Enhanced error messaging in the Precommit function for better debugging.
  • Updated the InitGenesis function for improved block processing.
  • Modified the Blockchain interface and blockchain struct for better block handling and validation.
  • Improved block insertion and processing logic in the ChainWriter interface.

New Features:

  • Added HasBlockAndState function to check the existence of a block and its state.
  • Introduced validator field in the blockchain struct for block validation.

Bug Fixes:

  • Removed a debug log statement from the SubscribeLogsEvent function for cleaner logs.

Tests:

  • Increased the number of blocks to wait for in the "Distribution Precompile" test for more reliable results.

Chores:

  • Updated import statements and added new methods to interfaces for better code organization and functionality.

@itsdevbear itsdevbear requested review from ocnc and calbera November 1, 2023 18:15
Copy link

coderabbitai bot commented Nov 1, 2023

Warning

Rate Limit Exceeded

@itsdevbear has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 7 minutes and 40 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per repository.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between f981084 and a3b3d43.

Walkthrough

The changes revolve around enhancing the blockchain and EVM keeper functionalities in the Polaris project. Key modifications include improved block processing and validation, addition of new methods to the Blockchain interface, and changes to the Miner struct. The InitGenesis function has been updated, and the error reporting in the Precommit function has been made more detailed.

Changes

File(s) Summary
cosmos/x/evm/keeper/abci.go Updated the panic message in the Precommit function to include more detailed information about the failed block.
cosmos/x/evm/keeper/genesis.go Modified the InitGenesis function, moving some lines outside the chain and replacing others.
cosmos/x/evm/keeper/keeper.go Import statements updated, new methods added to the Blockchain interface, and some methods removed.
eth/core/chain.go Introduced a new field validator to the blockchain struct.
eth/core/chain_reader.go Removed a TODO comment in the CurrentSafeBlock function.
eth/core/chain_resources.go Introduced a new function HasBlockAndState to the blockchain struct.
eth/core/chain_writer.go Modified block insertion and processing logic, added validation for the genesis block, and introduced changes related to storing block headers and historical data.
eth/polar/api_backend.go Removed a debug log statement from the SubscribeLogsEvent function.
cosmos/miner/miner.go Added new interfaces and fields, and modified the New and PrepareProposal functions.
cosmos/runtime/runtime.go Imported the context package, added new methods and interfaces, and modified the Build function.
e2e/precompile/contracts/distribution/distribution_test.go Increased the number of blocks to wait for in the "Distribution Precompile" test.
e2e/testapp/app.go Modified the NewPolarisApp function.

🐇🍂 "As the leaves fall, so do the old ways,
In the heart of autumn, we embrace new days.
With each line of code, we weave a tale,
On the ship of progress, we set sail.
Celebrate the changes, for they bring light,
To the darkest corners of the code's night.
So here's to the season, crisp and bright,
And to the coders, coding into the night!" 🌙💻


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between b5236fe and 25f11a7.
Files selected for processing (8)
  • cosmos/x/evm/keeper/abci.go (1 hunks)
  • cosmos/x/evm/keeper/genesis.go (1 hunks)
  • cosmos/x/evm/keeper/keeper.go (1 hunks)
  • eth/core/chain.go (4 hunks)
  • eth/core/chain_reader.go (1 hunks)
  • eth/core/chain_resources.go (1 hunks)
  • eth/core/chain_writer.go (2 hunks)
  • eth/polar/api_backend.go (1 hunks)
Files skipped from review due to trivial changes (3)
  • cosmos/x/evm/keeper/abci.go
  • eth/core/chain_reader.go
  • eth/polar/api_backend.go
Additional comments: 12
cosmos/x/evm/keeper/genesis.go (1)
  • 44-49: The changes in this hunk seem to be in line with the pull request's summary. The PreparePlugins method is now called directly on k.chain instead of being nested within another method. This improves readability and makes the code more straightforward. The replacement of InsertBlockWithoutSetHead with InsertGenesisBlock is also mentioned in the summary. Ensure that the new InsertGenesisBlock method correctly handles all the cases that the old method did.
eth/core/chain.go (2)
  • 66-72: The validator field has been added to the blockchain struct and is initialized in the NewChain function. Ensure that the validator is used correctly throughout the codebase.

  • 132-135: The validator is initialized with a new instance of core.BlockValidator. This is a good practice as it ensures that the validator is always in a valid state when a blockchain instance is created.

cosmos/x/evm/keeper/keeper.go (2)
  • 31-37: The import statements have been updated. Ensure that the new imports are used correctly in the code and the removed import is not used anywhere.

  • 39-45: The Blockchain interface has been updated. Make sure all implementations of this interface are updated accordingly.

- core.ChainWriter
- core.ChainReader
+ InsertGenesisBlock(*coretypes.Block) error
+ InsertBlockWithoutSetHead(*coretypes.Block) error
+ GetBlockByNumber(uint64) *coretypes.Block
eth/core/chain_writer.go (7)
  • 22-32: The import statements are well organized and separated by functionality. Good practice.

  • 34-41: The ChainWriter interface has been updated to include new methods and remove core.ChainWriter and core.ChainReader. Ensure that all implementations of this interface have been updated accordingly.

  • 44-50: The InsertGenesisBlock function now includes a validation check for the genesis block. This is a good practice to ensure the integrity of the genesis block.

  • 52-80: The InsertBlockWithoutSetHead function has been updated to include block validation and processing logic. This is a good practice to ensure the integrity of the blocks being inserted into the blockchain.

  • 82-132: The WriteBlockAndSetHead function has been updated to include block validation, processing logic, and storing block headers and historical data. This is a good practice to ensure the integrity of the blocks being inserted into the blockchain and the consistency of the blockchain state.

  • 134-165: The writeBlockWithState function is a new function introduced to write the block and its state to the blockchain. This is a good practice to ensure the integrity of the blocks being inserted into the blockchain and the consistency of the blockchain state.

  • 22-178: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [167-193]

The writeHistoricalData function is a new function introduced to store block headers and historical data. This is a good practice to ensure the integrity of the blocks being inserted into the blockchain and the consistency of the blockchain state.

eth/core/chain_resources.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 25f11a7 and 7fa7794.
Files selected for processing (1)
  • eth/core/chain_writer.go (2 hunks)
Additional comments: 5
eth/core/chain_writer.go (5)
  • 34-41: The ChainWriter interface has been updated to include the InsertGenesisBlock and InsertBlockWithoutSetHead methods. Ensure that all implementations of this interface have been updated accordingly.

  • 44-51: The InsertGenesisBlock function now checks if the block being inserted is the genesis block (i.e., its number is 0). If not, it returns an error. This is a good safety check to prevent incorrect usage of this function.

  • 54-83: The InsertBlockWithoutSetHead function has been significantly updated. It now validates the block body, processes the EVM block, and validates the state after block processing. If any of these steps fail, it logs an error and returns the error. This is a good improvement in terms of safety and error handling.

  • 137-168: The writeBlockWithState function writes the block and its state to the blockchain. It also logs an error if storing the block header or writing historical data fails. This function seems to be well-structured and handles errors appropriately.

  • 22-181: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [170-196]

The writeHistoricalData function writes the block, receipts, and transactions to the host chain if the historical plugin is supported. It logs an error and returns it if any of these operations fail. This function seems to be well-structured and handles errors appropriately.

eth/core/chain_writer.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #1266 (08e11ad) into main (b5236fe) will decrease coverage by 0.50%.
The diff coverage is 0.00%.

❗ Current head 08e11ad differs from pull request most recent head a3b3d43. Consider uploading reports for the commit a3b3d43 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1266      +/-   ##
==========================================
- Coverage   49.23%   48.73%   -0.50%     
==========================================
  Files          84       84              
  Lines        4789     4838      +49     
==========================================
  Hits         2358     2358              
- Misses       2257     2306      +49     
  Partials      174      174              
Files Coverage Δ
cosmos/x/evm/keeper/keeper.go 0.00% <ø> (ø)
eth/core/chain_reader.go 0.00% <ø> (ø)
cosmos/x/evm/keeper/processor.go 0.00% <0.00%> (ø)
cosmos/x/evm/module.go 21.42% <0.00%> (ø)
eth/core/chain.go 0.00% <0.00%> (ø)
cosmos/x/evm/keeper/genesis.go 0.00% <0.00%> (ø)
cosmos/x/evm/keeper/abci.go 0.00% <0.00%> (ø)
eth/core/chain_resources.go 0.00% <0.00%> (ø)
eth/core/chain_writer.go 0.00% <0.00%> (ø)

eth/core/chain_writer.go Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 7fa7794 and cb37dfa.
Files selected for processing (5)
  • cosmos/x/evm/keeper/genesis.go (1 hunks)
  • cosmos/x/evm/keeper/keeper.go (1 hunks)
  • cosmos/x/evm/keeper/processor.go (1 hunks)
  • eth/core/chain.go (4 hunks)
  • eth/core/chain_writer.go (2 hunks)
Additional comments: 14
cosmos/x/evm/keeper/keeper.go (3)
  • 31-37: The import statements have been updated to reflect the new dependencies. Ensure that these packages exist and are accessible.

  • 39-45: The Blockchain interface has been updated. Ensure that all implementations of this interface have been updated to reflect these changes.

  • 47-47: The Keeper struct remains unchanged.

cosmos/x/evm/keeper/processor.go (1)
  • 64-66: The InsertBlockAndSetHead function is now being used instead of InsertBlockWithoutSetHead. Ensure that this change is intentional and that the new function behaves as expected in all scenarios where it is used.
cosmos/x/evm/keeper/genesis.go (1)
  • 44-49: The changes to the InitGenesis function look good. The addition of k.chain.PreparePlugins(ctx.WithEventManager(sdk.NewEventManager())) ensures that the plugins are prepared with a new event manager context before writing the genesis block. This could improve the efficiency and reliability of the function. However, ensure that the PreparePlugins and WriteGenesisBlock functions handle errors appropriately internally, as they are not checked here.
eth/core/chain.go (3)
  • 66-72: The addition of the validator field and the removal of currentReceipts and currentLogs fields are noted. Ensure that all references to these fields in the codebase have been updated accordingly. The validator field should be initialized properly in the constructor.

  • 80-85: The currentBlock and finalizedBlock fields have been updated to use atomic.Pointer. This is a good practice for concurrent programming to avoid data races. However, ensure that all operations on these fields are done using atomic operations.

  • 95-105: The logger field has been updated to type log.Logger. Ensure that all logging operations in the codebase have been updated to use this new type.

eth/core/chain_writer.go (6)
  • 34-41: The ChainWriter interface has been updated to include the WriteGenesisBlock and InsertBlockAndSetHead methods. Ensure that all implementations of this interface have been updated accordingly.

  • 44-51: The WriteGenesisBlock method has been added to the blockchain struct. It writes the genesis block and sets it as the head. The genesis block is validated to ensure its block number is 0. If not, an error is returned. The method then calls WriteBlockAndSetHead to write the block and set it as the head. If an error occurs during this process, it is returned.

  • 53-83: The InsertBlockAndSetHead method has been added to the blockchain struct. It validates the block body, processes the EVM block, validates the state after processing, and then writes the block and sets it as the head. If an error occurs at any point, it is logged and returned.

  • 85-151: The WriteBlockAndSetHead method has been updated to include the emitHeadEvent parameter and additional logic for setting the current block and finalizing the block. It writes the block to the store, validates the parent block, sets the current block, finalizes the block, stores txLookup entries for all transactions in the block, writes the receipts cache, and fires off the feeds. If emitHeadEvent is true, it fires a ChainHeadEvent.

  • 153-184: The writeBlockWithState method writes the block along with its state (receipts and logs) into the blockchain. It stores the header, writes the block to the database, commits all cached state changes into the underlying memory database, and logs the finalization of the EVM block. If an error occurs at any point, it is logged and returned.

  • 22-197: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [186-212]

The writeHistoricalData method writes the block, receipts, and transactions to the host chain if the historical plugin is supported. If an error occurs at any point, it is logged and returned.

cosmos/x/evm/keeper/processor.go Show resolved Hide resolved
eth/core/chain.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between cb37dfa and 57ff739.
Files selected for processing (6)
  • cosmos/miner/miner.go (3 hunks)
  • cosmos/runtime/runtime.go (3 hunks)
  • cosmos/x/evm/keeper/abci.go (1 hunks)
  • e2e/precompile/contracts/distribution/distribution_test.go (1 hunks)
  • e2e/testapp/app.go (1 hunks)
  • eth/core/chain_writer.go (2 hunks)
Files skipped from review due to trivial changes (1)
  • cosmos/x/evm/keeper/abci.go
Additional comments: 16
e2e/testapp/app.go (1)
  • 191-196: The app.Polaris.Build function call has been modified to pass app instead of app.BaseApp as the first argument. This change may indicate a modification in the way the Build function is implemented or how it interacts with the app object. Ensure that the Build function can handle the entire app object and that this change doesn't introduce any unintended side effects.
- if err := app.Polaris.Build(app.BaseApp, app.EVMKeeper); err != nil {
+ if err := app.Polaris.Build(app, app.EVMKeeper); err != nil {
    panic(err)
  }
cosmos/runtime/runtime.go (4)
  • 21-27: The context package is imported. Ensure that it is used in the codebase.

  • 51-55: The PrepareCheckState method is added to the EVMKeeper interface. Make sure all implementations of this interface have this method.

  • 58-63: The miner.App interface is embedded in the CosmosApp interface. This will add all the methods of miner.App to CosmosApp. Ensure that this is the intended behavior.

  • 101-111: The miner.New function is called with an additional argument app of type miner.App. Ensure that the miner.New function has been updated to accept this new argument.

cosmos/miner/miner.go (6)
  • 32-38: The new import "pkg.berachain.dev/polaris/cosmos/x/evm/keeper" is added. Ensure that the package is available and accessible.

  • 49-51: The new interface "App" is added with the method "BeginBlocker". Ensure that all implementations of this interface have this method.

  • 53-58: The new interface "EVMKeeper" is added with the methods "Setup" and "PrepareCheckState". Ensure that all implementations of this interface have these methods.

  • 63-66: The new field "app" of type "App" is added to the struct "Miner". Ensure that all instances of "Miner" are updated to include this field.

  • 69-74: The function "New" now takes an additional parameter "app App". Ensure that all calls to this function are updated to include this parameter.

  • 84-96: The method "PrepareProposal" now calls "app.BeginBlocker". Ensure that the "BeginBlocker" method is implemented correctly in the "App" interface.

eth/core/chain_writer.go (5)
  • 34-41: The ChainWriter interface has been updated to include the WriteGenesisBlock and InsertBlockAndSetHead methods. Ensure that all implementations of this interface have been updated accordingly.

  • 44-51: The WriteGenesisBlock function has been added to the blockchain struct. It checks if the block number is 0 (which is expected for a genesis block) and then calls WriteBlockAndSetHead. The error handling is appropriate.

  • 88-153: The WriteBlockAndSetHead function has been updated to include an additional parameter emitHeadEvent. It writes the block to the store, checks if the parent is the head block, sets the current block, finalizes the block, updates the caches, and fires off the feeds. The error handling is appropriate.

  • 156-186: The writeBlockWithState function has been added. It writes the block header, the block itself, commits all cached state changes, and logs the finalization of the EVM block. The error handling is appropriate.

  • 22-199: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [189-213]

The writeHistoricalData function has been added. It stores the block, receipts, and transactions if the historical plugin is supported. The error handling is appropriate.

Comment on lines +124 to 127
// Wait for 5 blocks to be produced, to make sure there are rewards.
for i := 0; i < 5; i++ {
Expect(tf.WaitForNextBlock()).To(Succeed())
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from waiting for 2 blocks to 5 blocks is reasonable if it ensures that there are enough blocks produced to have rewards. However, it might increase the test execution time. If the test suite has many tests that wait for blocks, this could significantly slow down the overall test execution. Consider if there's a more efficient way to ensure that enough blocks are produced, such as by mocking or simulating block production.

Comment on lines 54 to 85
// For now, it is a huge lie. It does infact set the head.
func (bc *blockchain) InsertBlockAndSetHead(block *types.Block) error {
// Validate that we are about to insert a valid block.
if block.NumberU64() > 1 { // TODO DIAGNOSE
if err := bc.validator.ValidateBody(block); err != nil {
log.Error("invalid block body", "err", err)
return err
}
}

// Process block using the parent state as reference point
pstart := time.Now()
receipts, logs, _, err := bc.processor.Process(block, bc.statedb, *bc.vmConfig)
// Process the incoming EVM block.
receipts, logs, usedGas, err := bc.processor.Process(block, bc.statedb, *bc.vmConfig)
if err != nil {
log.Error("failed to process block", "num", block.NumberU64(), "err", err)
return err
}
ptime := time.Since(pstart)
bc.logger.Info("processed block in", "time", ptime)
return bc.InsertBlock(block, receipts, logs)
}

// InsertBlock inserts a block into the canonical chain and updates the state of the blockchain.
func (bc *blockchain) InsertBlock(
block *types.Block,
receipts types.Receipts,
logs []*types.Log,
) error {
var err error
if _, err = bc.statedb.Commit(
block.NumberU64(),
bc.config.IsEIP158(block.Header().Number),
); err != nil {
// ValidateState validates the statedb post block processing.
if err = bc.validator.ValidateState(block, bc.statedb, receipts, usedGas); err != nil {
log.Error("invalid state after processing block", "num", block.NumberU64(), "err", err)
return err
}

// TODO: prepare historical plugin here?
// TBH still think we should deprecate it and run in another routine as indexer.
// We can just immediately finalize the block. It's okay in this context.
if _, err = bc.WriteBlockAndSetHead(
block, receipts, logs, nil, true); err != nil {
log.Error("failed to write block", "num", block.NumberU64(), "err", err)
return err
}

// ***************************************** //
// TODO: add safety check for canonicallness //
// ***************************************** //
return err
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The InsertBlockAndSetHead function has been added. It validates the block, processes it, validates the state after processing, and then writes the block. The error handling is appropriate. However, the comment on line 55 is misleading and should be updated to reflect the actual behavior of the function.

- // For now, it is a huge lie. It does infact set the head.
+ // This function inserts a block and also sets it as the head.

Commitable suggestion (Beta)
Suggested change
// For now, it is a huge lie. It does infact set the head.
func (bc *blockchain) InsertBlockAndSetHead(block *types.Block) error {
// Validate that we are about to insert a valid block.
if block.NumberU64() > 1 { // TODO DIAGNOSE
if err := bc.validator.ValidateBody(block); err != nil {
log.Error("invalid block body", "err", err)
return err
}
}
// Process block using the parent state as reference point
pstart := time.Now()
receipts, logs, _, err := bc.processor.Process(block, bc.statedb, *bc.vmConfig)
// Process the incoming EVM block.
receipts, logs, usedGas, err := bc.processor.Process(block, bc.statedb, *bc.vmConfig)
if err != nil {
log.Error("failed to process block", "num", block.NumberU64(), "err", err)
return err
}
ptime := time.Since(pstart)
bc.logger.Info("processed block in", "time", ptime)
return bc.InsertBlock(block, receipts, logs)
}
// InsertBlock inserts a block into the canonical chain and updates the state of the blockchain.
func (bc *blockchain) InsertBlock(
block *types.Block,
receipts types.Receipts,
logs []*types.Log,
) error {
var err error
if _, err = bc.statedb.Commit(
block.NumberU64(),
bc.config.IsEIP158(block.Header().Number),
); err != nil {
// ValidateState validates the statedb post block processing.
if err = bc.validator.ValidateState(block, bc.statedb, receipts, usedGas); err != nil {
log.Error("invalid state after processing block", "num", block.NumberU64(), "err", err)
return err
}
// TODO: prepare historical plugin here?
// TBH still think we should deprecate it and run in another routine as indexer.
// We can just immediately finalize the block. It's okay in this context.
if _, err = bc.WriteBlockAndSetHead(
block, receipts, logs, nil, true); err != nil {
log.Error("failed to write block", "num", block.NumberU64(), "err", err)
return err
}
// ***************************************** //
// TODO: add safety check for canonicallness //
// ***************************************** //
return err
}
// This function inserts a block and also sets it as the head.
func (bc *blockchain) InsertBlockAndSetHead(block *types.Block) error {
// Validate that we are about to insert a valid block.
if block.NumberU64() > 1 { // TODO DIAGNOSE
if err := bc.validator.ValidateBody(block); err != nil {
log.Error("invalid block body", "err", err)
return err
}
}
// Process the incoming EVM block.
receipts, logs, usedGas, err := bc.processor.Process(block, bc.statedb, *bc.vmConfig)
if err != nil {
log.Error("failed to process block", "num", block.NumberU64(), "err", err)
return err
}
// ValidateState validates the statedb post block processing.
if err = bc.validator.ValidateState(block, bc.statedb, receipts, usedGas); err != nil {
log.Error("invalid state after processing block", "num", block.NumberU64(), "err", err)
return err
}
// We can just immediately finalize the block. It's okay in this context.
if _, err = bc.WriteBlockAndSetHead(
block, receipts, logs, nil, true); err != nil {
log.Error("failed to write block", "num", block.NumberU64(), "err", err)
return err
}
return err
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 57ff739 and ed2fd90.
Files selected for processing (2)
  • cosmos/miner/miner.go (3 hunks)
  • cosmos/runtime/runtime.go (3 hunks)
Additional comments: 9
cosmos/runtime/runtime.go (4)
  • 21-27: The import of the context package is new. Ensure that it is used appropriately in the code.

  • 51-55: The EVMKeeper interface has a new method PrepareCheckState. Ensure that all implementations of this interface have this method.

  • 58-63: The CosmosApp interface now embeds the miner.App interface. Ensure that all implementations of CosmosApp are compatible with this change.

  • 105-107: The WrappedMiner field in the Polaris struct is modified to include the app and ek parameters in the miner.New function call. Ensure that this change is compatible with the miner.New function definition.

cosmos/miner/miner.go (5)
  • 32-38: The new import evmkeeper "pkg.berachain.dev/polaris/cosmos/x/evm/keeper" is added. Ensure that the package is available and accessible.

  • 46-58: The new interfaces App and EVMKeeper are introduced. Make sure that these interfaces are implemented wherever required.

  • 61-67: Two new fields app and keeper are added to the Miner struct. Ensure that these fields are properly initialized and used.

  • 70-76: The New function is modified to accept app and keeper parameters. Make sure that all calls to this function are updated to match the new signature.

  • 84-103: The PrepareProposal function is updated to call PrepareCheckState and BeginBlocker methods. Ensure that these methods are correctly implemented and that they return the expected results.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ed2fd90 and f981084.
Files selected for processing (4)
  • cosmos/miner/miner.go (3 hunks)
  • cosmos/runtime/runtime.go (3 hunks)
  • cosmos/x/evm/keeper/abci.go (2 hunks)
  • cosmos/x/evm/module.go (1 hunks)
Additional comments: 13
cosmos/x/evm/keeper/abci.go (2)
  • 35-43: The error handling has been improved to provide more detailed information about the failed block. This is a good practice as it aids in debugging and understanding the system's behavior.

  • 52-56: The function PrepareCheckState has been renamed to SetLatestQueryContext. Ensure that all calls to this function throughout the codebase have been updated to match the new name.

cosmos/x/evm/module.go (1)
  • 126-132: The function PrepareCheckState has been renamed to SetLatestQueryContext and now returns an error. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and handle the returned error appropriately.
- func (am AppModule) PrepareCheckState(ctx context.Context) {
- 	am.keeper.SetLatestQueryContext(ctx)
- }
+ func (am AppModule) SetLatestQueryContext(ctx context.Context) error {
+ 	return am.keeper.SetLatestQueryContext(ctx)
+ }
cosmos/runtime/runtime.go (4)
  • 21-27: The import of the context package is added. Ensure that it is used in the code.

  • 51-55: The EVMKeeper interface is extended with a new method SetLatestQueryContext. Make sure that all implementations of this interface are updated to include this method.

  • 58-63: The CosmosApp interface now extends the miner.App interface. Ensure that all implementations of CosmosApp are updated to include the methods from miner.App.

  • 101-111: The Build function now takes an additional app argument of type CosmosApp and an additional ek argument of type EVMKeeper. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

cosmos/miner/miner.go (6)
  • 32-38: The new import statement looks fine. Ensure that the package "pkg.berachain.dev/polaris/cosmos/x/evm/keeper" is available and accessible.

  • 49-52: The new interface App with methods BeginBlocker and PreBlocker is introduced. Ensure that the methods are implemented wherever this interface is used.

  • 54-59: The new interface EVMKeeper with methods Setup and SetLatestQueryContext is introduced. Ensure that the methods are implemented wherever this interface is used.

  • 62-68: The Miner struct now includes two new fields: app of type App and keeper of type EVMKeeper. Ensure that these fields are properly initialized and used.

  • 70-77: The New function now accepts two additional parameters: app of type App and keeper of type EVMKeeper. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

  • 85-117: The PrepareProposal function has been modified to include additional logic related to the new interfaces and methods. Ensure that the logic is correct and that error handling is properly done.

cosmos/runtime/runtime.go Show resolved Hide resolved
@@ -68,11 +85,34 @@ func (m *Miner) Init(serializer EnvelopeSerializer) {
func (m *Miner) PrepareProposal(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderbez is this safe?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't dug into the specifics of each individual method call, but since PrepareProposal state is discarded upon round timeout, any state modifications should be safe assuming you use the provided ctx (which it looks like you are!).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd like, I can dive into the individual method calls for further analysis.

@itsdevbear itsdevbear merged commit 949a826 into main Nov 1, 2023
7 checks passed
@itsdevbear itsdevbear deleted the safety-checks branch November 1, 2023 20:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants